Fix npmnpmrcproject fixture: scope legacy auth keys to the registry - #3681
Open
agrasth wants to merge 2 commits into
Open
Fix npmnpmrcproject fixture: scope legacy auth keys to the registry#3681agrasth wants to merge 2 commits into
agrasth wants to merge 2 commits into
Conversation
npm 12 hard-rejects the bare (unscoped) `email` and `_auth` auth config keys with ERR_INVALID_AUTH — a validation change from npm's own auth-config cleanup, not a jfrog-cli npm command bug. Every npm 12.x compatibility test failed at the very first `npm install` call in TestNpmNativeSyntax/npm_i_with_npmrc_project because the fixture's .npmrc still used the old unscoped format: _auth=YWRtaW46QVBFG1ZkZFMzN3NCakJiaRFVBThVb0JlZzFl email=ddd@dd.dd npm's own error message names the fix: scope both keys to the registry, matching how the existing _authToken line is already scoped: //NO-NO-REPO/:_auth=YWRtaW46QVBFG1ZkZFMzN3NCakJiaRFVBThVb0JlZzFl //NO-NO-REPO/:email=ddd@dd.dd Verified locally with the exact npm 12.0.0 that failed in CI — `npm install --dry-run` against the fixed .npmrc no longer raises ERR_INVALID_AUTH (it fails only on ENOTFOUND for the fixture's fake NO-NO-REPO host, which is expected and unrelated: the real test always redirects to a live Artifactory instance via jfrog-cli).
golangci-lint (staticcheck SA1019) started failing on this PR's CI run because master recently bumped to Go 1.26, which deprecates httputil.ReverseProxy.Director in favor of Rewrite (available since Go 1.20). Unrelated to the npm fixture change in this PR, but it blocks the CI gate regardless, so fixing it here. Direct translation: Rewrite receives a *httputil.ProxyRequest whose .Out field is a pre-cloned copy of the incoming request, so the closure only needs to set the same three fields Director set directly on the request (Host, URL.Host, URL.Scheme). Defining a custom Rewrite func means none of the automatic default behavior (X-Forwarded-For, etc.) kicks in, matching Director's original no-defaults behavior exactly -- this is a mechanical migration with no functional change. Verified with the exact CI lint invocation (golangci-lint 2.13.1, same flag set) against both the changed package and the full repo: 0 issues.
Contributor
|
View full scan results in JFrog Platform📗 Scan Summary
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| pr.Out.URL.Scheme = target.Scheme | ||
| } | ||
| tr := &http.Transport{ | ||
| TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, |
Contributor
There was a problem hiding this comment.
🎯 Static Application Security Testing (SAST) Vulnerability
| Severity | Finding |
|---|---|
 High |
TLS settings are configured insecurely, exposing communications to risks |
Full description
Vulnerability Details
| Rule ID: | go-insecure-tls |
Overview
Insecure TLS Configuration is a type of vulnerability that occurs when an
application uses weak or outdated cryptographic protocols, ciphers, or
configurations for secure communication over the network.
Vulnerable example
package main
import (
"crypto/tls"
)
func main() {}
func insecureMinMaxTlsVersion() {
{
config := &tls.Config{}
config.MinVersion = 0
}
{
config := &tls.Config{}
config.MinVersion = tls.VersionSSL30
}
{
config := &tls.Config{}
config.MaxVersion = tls.VersionSSL30
}
{
config := &tls.Config{}
}
}
func insecureCipherSuites() {
config := &tls.Config{
CipherSuites: []uint16{
tls.TLS_RSA_WITH_RC4_128_SHA,
},
}
_ = config
}In this example, the MinVersion field is set to tls.VersionSSL30, which
uses the outdated SSL 3.0 protocol, making the application vulnerable to
attacks such as POODLE.
Remediation
package main
import (
"crypto/tls"
)
func main() {}
func insecureMinMaxTlsVersion() {
{
config := &tls.Config{}
- config.MinVersion = 0
+ config.MinVersion = tls.VersionTLS12
}
{
config := &tls.Config{}
- config.MinVersion = tls.VersionSSL30
+ config.MinVersion = tls.VersionTLS12
}
{
config := &tls.Config{}
- config.MaxVersion = tls.VersionSSL30
}
{
- config := &tls.Config{}
+ config := &tls.Config{MinVersion: tls.VersionTLS12}
}
}
func insecureCipherSuites() {
config := &tls.Config{
CipherSuites: []uint16{
- tls.TLS_RSA_WITH_RC4_128_SHA,
+ tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
},
+ MinVersion: tls.VersionTLS12,
}
_ = config
}By using safe TLS versions (e.g., tls.VersionTLS12) and secure cipher suites we can
mitigate the risk of insecure TLS configurations and improve the security of the
application.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
emailand_authnpmrc auth config keys withERR_INVALID_AUTH, which was failingTestNpmNativeSyntax/npm_i_with_npmrc_project(and the equivalent legacy-syntax test) for every npm 12.x version in our pm-version-monitor compatibility tracking — the very firstnpm installcall in the test errored out before jfrog-cli's own npm command logic ever ran.testdata/npm/npmnpmrcproject/.npmrcstill used the pre-npm-12 unscoped auth key format._authTokenline already uses (and exactly what npm's own error message names as the fix).Test plan
npm install --dry-runagainst the original fixture with npm 12.0.0 reproducesERR_INVALID_AUTH: Invalid auth configuration found: 'email' must be renamed to '//<registry>/:email' in project configENOTFOUNDfor the fixture's placeholderNO-NO-REPOhost, which is expected — the real test redirects to a live Artifactory instance)go vet ./...andgofmt— no changes needed, fixture-only diff